Feat/cloud fn test #577
Feat/cloud fn test #577
Conversation
| try { | ||
| return JSON.stringify(obj, null, 2); | ||
| } catch { | ||
| return '[Unserializable Object]'; |
There was a problem hiding this comment.
Things with circular refs, rarely happen, but they cause JSON.stringify failure
packages/smtppostmaster/src/index.ts
Outdated
| const resolvedSecure = secureFromEnv ?? resolvedPort === 465; | ||
|
|
||
| const user = process.env.SMTP_USER; | ||
| const pass = process.env.SMTP_PASS; |
There was a problem hiding this comment.
can we move all of the process.env into our environment system instead of inlined?
I think pgpm/env or pgpm/types and we can use the defaults system, too
There was a problem hiding this comment.
there should be no process.env in any modules — all hoisted into env/types (like telescope)
packages/smtppostmaster/src/index.ts
Outdated
| maxMessages?: number; | ||
| }; | ||
|
|
||
| const parseNumberFromEnv = (name: string) => { |
There was a problem hiding this comment.
if you hoist the types/env stuff you can likely get rid of this too, and it looks duplicative of stuff we already have in the getEnvOptions and family
packages/smtppostmaster/src/index.ts
Outdated
| @@ -0,0 +1,164 @@ | |||
| import nodemailer, { SendMailOptions } from 'nodemailer'; | |||
| import SMTPTransport from 'nodemailer/lib/smtp-transport'; | |||
| import { parseEnvBoolean, parseEnvNumber } from '@pgpmjs/env'; | |||
There was a problem hiding this comment.
instead of importing these low-level methods, we should just be calling one of the getEnv here
| const catcher = useCatcher ? await createSmtpCatcher() : null; | ||
|
|
||
| if (catcher) { | ||
| process.env.SMTP_HOST = catcher.host; |
There was a problem hiding this comment.
similar advice? (regarding many env vars)
I guess for tests it would be slightly more acceptable, but maybe we can just use overrides and call a getEnv from @pgpm/env or @pgpm/types
| import { send } from '../src/index'; | ||
| import { createSmtpCatcher } from './smtp-catcher'; | ||
|
|
||
| const applyEnv = (overrides: Record<string, string | undefined>) => { |
There was a problem hiding this comment.
let's not do this and properly use the env vars from types/env packages
- Add SmtpOptions interface and defaults to @pgpmjs/types - Add SMTP_* env var parsing to @pgpmjs/env getEnvVars() - Refactor smtppostmaster to use getEnvOptions() instead of direct process.env - Add smtpOverrides parameter to send() for programmatic configuration - Add resetTransport() helper for test isolation - Update tests to use smtpOverrides instead of env manipulation - Update README with new programmatic override examples Addresses PR #577 feedback to move process.env access into the centralized environment system.
…v-refactor Devin/1768714819 smtp env refactor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
|
|
||
| return { transport, smtpOpts: cachedSmtpOpts ?? smtpOpts }; | ||
| }; |
There was a problem hiding this comment.
SMTP transport cache persists overrides unexpectedly
Medium Severity
The getTransport function overwrites the module-level transport singleton whenever smtpOverrides is provided. Subsequent calls to send() without overrides will reuse this cached transport, which was configured with the previous override values. This means if someone calls send({...}, { host: 'custom.smtp.com' }) once, all subsequent calls to send({...}) without overrides will continue using custom.smtp.com instead of the environment-configured SMTP server, until resetTransport() is explicitly called.
| } | ||
|
|
||
| return this.result; | ||
| } |
There was a problem hiding this comment.
Missing cleanup on partial function startup failure
Medium Severity
When startFunctions() fails partway through (e.g., second function fails due to port conflict), previously started function servers remain running with no cleanup. The start() method sets this.started = true before starting functions, then if startFunctions() throws, the error propagates without closing the already-started servers in functionServers. These HTTP servers continue running as orphaned resources until process termination. A try/catch in start() that calls cleanup on failure is missing.
Summary
knative-job-service(callback server, worker/scheduler, and functions) and wirecnc jobs upto it.smtppostmasterpackage and improve logging/diagnostics.Changes
knative-job-serviceto the primary jobs runtime (KnativeJobsSvc) with env-based boot helpers and function orchestration.jobs/knative-job-service, start GraphQL server in-test, and add failure scenarios.@constructive-io/smtppostmasterand updatesimple-email/send-email-linkto use SMTP.cnc jobs upto knative-job-serviceTesting
pnpm --filter @constructive-io/knative-job-service test -- jobs.e2e.test.tsNote
Introduces a unified jobs runtime and optional SMTP email sending with improved logging and shutdown handling.
jobs/knative-job-service(KnativeJobsSvc): orchestratescallbackserver, worker, scheduler, and function services; adds env-based boot helpers and graceful stop/UNLISTEN cleanupcnc jobs upcommand wired toknative-job-service@constructive-io/smtppostmasterpackage; functionssimple-emailandsend-email-linkcan switch viaEMAIL_SEND_USE_SMTP@pgpmjs/logger(addscreateLogger, better formatting) across jobs/functions/serversHttpServer, add listen/close lifecycle, cache cleanup, pg/graphile cache clear hooks@pgpmjs/env(+parseEnvNumber, SMTP config) and@pgpmjs/types(SmtpOptions, defaults)jobs.e2e.test.tswith seeded DB; spins up GraphQL + jobs + functions; covers success, failure, retries, and mail provider errors; CI runsjobs/knative-job-service@constructive-io/knative-job-fntocreateJobApp, improved callback/error handling; worker/scheduler/job-pg add graceful shutdownWritten by Cursor Bugbot for commit ada5524. This will update automatically on new commits. Configure here.